Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Notifications assets full support #2295

Merged
merged 12 commits into from
Apr 1, 2022

Conversation

danielayala94
Copy link
Contributor

No description provided.

@danielayala94 danielayala94 added the area-Notifications Toast notification, badges, Live Tiles, push notifications label Mar 18, 2022
@ghost ghost added the needs-triage label Mar 18, 2022
jonwis
jonwis previously requested changes Mar 18, 2022
dev/AppNotifications/AppNotificationUtility.cpp Outdated Show resolved Hide resolved
dev/AppNotifications/AppNotificationUtility.cpp Outdated Show resolved Hide resolved
dev/AppNotifications/AppNotificationUtility.cpp Outdated Show resolved Hide resolved
dev/AppNotifications/AppNotificationUtility.cpp Outdated Show resolved Hide resolved
dev/AppNotifications/AppNotificationUtility.cpp Outdated Show resolved Hide resolved
dev/AppNotifications/AppNotificationUtility.cpp Outdated Show resolved Hide resolved
dev/AppNotifications/AppNotificationUtility.cpp Outdated Show resolved Hide resolved
dev/AppNotifications/AppNotificationUtility.cpp Outdated Show resolved Hide resolved
dev/AppNotifications/AppNotificationUtility.cpp Outdated Show resolved Hide resolved
dev/AppNotifications/AppNotificationUtility.cpp Outdated Show resolved Hide resolved
dev/AppNotifications/AppNotificationUtility.cpp Outdated Show resolved Hide resolved
dev/AppNotifications/AppNotificationUtility.cpp Outdated Show resolved Hide resolved
dev/AppNotifications/AppNotificationUtility.cpp Outdated Show resolved Hide resolved
dev/AppNotifications/AppNotificationUtility.cpp Outdated Show resolved Hide resolved
dev/AppNotifications/AppNotificationUtility.cpp Outdated Show resolved Hide resolved
// subKey: \Software\Classes\AppUserModelId\{AppGUID}
std::wstring subKey{ c_appIdentifierPath + appId };

THROW_IF_WIN32_ERROR(RegCreateKeyEx(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does WIL have registry functions?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't believe it does. But that would be very nice to have :)

dev/AppNotifications/AppNotificationUtility.cpp Outdated Show resolved Hide resolved
@riverar
Copy link
Contributor

riverar commented Mar 19, 2022

I can't attach comments to unchanged lines so am putting it here instead.


GUID newNotificationGuid;
THROW_IF_FAILED(CoCreateGuid(&newNotificationGuid));
wil::unique_cotaskmem_string newNotificationGuidString;
THROW_IF_FAILED(StringFromCLSID(newNotificationGuid, &newNotificationGuidString));
RegisterValue(hKey, L"NotificationGUID", reinterpret_cast<const BYTE*>(newNotificationGuidString.get()), REG_SZ, wcslen(newNotificationGuidString.get()) * sizeof(wchar_t));
return newNotificationGuidString.get();

This chunk of code generates a GUID with a known length then measures the length again, which isn't efficient. Instead you can perhaps do one of a few things (consult your nearest FTE for final guidance):

  • Switch out the StringFromCLSID for StringFromGUID2, which returns the length
  • Switch out the wcslen and use a WIL guid length constant (i.e. wil::guid_string_length)
  • ...

displayName = wil::make_unique_string<wil::unique_cotaskmem_string>(SetDisplayNameBasedOnProcessName().c_str());

RegisterValue(hKey, L"DisplayName", reinterpret_cast<const BYTE*>(displayName.get()), REG_EXPAND_SZ, wcslen(displayName.get()) * sizeof(wchar_t));
RegisterValue(hKey, L"IconUri", reinterpret_cast<const BYTE*>(iconFilePath.get()), REG_EXPAND_SZ, wcslen(iconFilePath.get()) * sizeof(wchar_t));
RegisterValue(hKey, L"CustomActivator", reinterpret_cast<const BYTE*>(clsid.c_str()), REG_SZ, clsid.size() * sizeof(wchar_t));

SetDisplayNameBasedOnProcessName returns a std::wstring here (which tracks string length!), then we pull out a raw pointer, then measure the string length again. Maybe rejigger things a bit here?

@danielayala94 danielayala94 marked this pull request as ready for review April 1, 2022 17:25
@@ -175,7 +148,8 @@ int main()
const PACKAGE_VERSION minVersion{};
RETURN_IF_FAILED(MddBootstrapInitialize(c_Version_MajorMinor, nullptr, minVersion));

SetDisplayNameAndIcon();
// Not mandatory, but it's highly recommended to specify AppUserModelId
THROW_IF_FAILED(SetCurrentProcessExplicitAppUserModelID(L"TestAppId"));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vaheeshta This is needed if we want apps to maintain their toast states even after a process name update later in the future. For example, if unpackaged app foo.exe has a name change to bar.exe in the future, this ensures that the apps toasts remain in the actioncentre. We should document.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It also is needed if apps change their path from c:\foo\name.exe to d:\bar\name.exe.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it. I'll document this once I have the unpackaged sample code.

dev/AppNotifications/ShellLocalization.cpp Show resolved Hide resolved
dev/AppNotifications/ShellLocalization.cpp Outdated Show resolved Hide resolved
dev/AppNotifications/ShellLocalization.cpp Outdated Show resolved Hide resolved
dev/AppNotifications/ShellLocalization.cpp Outdated Show resolved Hide resolved
dev/AppNotifications/ShellLocalization.cpp Outdated Show resolved Hide resolved
dev/AppNotifications/ShellLocalization.cpp Outdated Show resolved Hide resolved
dev/AppNotifications/ShellLocalization.h Outdated Show resolved Hide resolved
dev/AppNotifications/ShellLocalization.h Outdated Show resolved Hide resolved
@danielayala94
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@danielayala94
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@danielayala94 danielayala94 dismissed stale reviews from DrusTheAxe and jonwis April 1, 2022 23:23

I have addressed all comments. Thank you for reviewing this PR! 😊

@danielayala94 danielayala94 merged commit 1006b2a into main Apr 1, 2022
@danielayala94 danielayala94 deleted the notifications-assets-full-support branch April 1, 2022 23:24

// Store the converted bitmap as ppToRenderBitmapSource
winrt::com_ptr<IWICBitmapSource> wicBitmapSourceConverted;
THROW_IF_FAILED(wicFormatConverter->QueryInterface(_uuidof(IWICBitmapSource), wicBitmapSourceConverted.put_void()));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(No change required)

I think this can be return wicFormatConverter.as<IWICBitmapSource>() ?


THROW_IF_FAILED(wicFormatConverter->Initialize(wicBitmapSource.get(), guidPixelFormatSource, bitmapDitherType, nullptr, 0.f, WICBitmapPaletteTypeCustom));

// Store the converted bitmap as ppToRenderBitmapSource
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dead comment, please remove

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-Notifications Toast notification, badges, Live Tiles, push notifications needs-triage
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants